Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Added the ability to get or set the last change tick of a system. #5838

Closed

Conversation

StarArawn
Copy link
Contributor

Objective

I'm build a UI system for bevy. In this UI system there is a concept of a system per UI entity. I had an issue where change detection wasn't working how I would expect and it's because when a function system is ran the last_change_tick is updated with the latest tick(from world). In my particular case I want to "wait" to update the last_change_tick until after my system runs for each entity.

Solution

Initially I thought bypassing the change detection all together would be a good fix, but on talking to some users in discord a simpler fix is to just expose last_change_tick to the end users. This is achieved by adding the following to the System trait:

    /// Allows users to get the system's last change tick.
    fn get_last_change_tick(&self) -> u32;
    /// Allows users to set the system's last change tick.
    fn set_last_change_tick(&mut self, last_change_tick: u32);

This causes a bit of weirdness with two implementors of System. FixedTimestep and ChainSystem both implement system and thus it's required that some sort of implementation be given for the new functions. I solved this by outputting a warning and not doing anything for these systems.

I think it's important to understand why I can't add the new functions only to the function system and not to the System trait. In my code I store the systems generically as Box<dyn System<...>>. I do this because I have differing parameters that are being passed in depending on the UI widget's system. As far as I can tell there isn't a way to take a system trait and cast it into a specific type without knowing what those parameters are.

In my own code this ends up looking something like:

// Runs per entity.
let old_tick = widget_system.get_last_change_tick();
should_update_children = widget_system.run((widget_tree.clone(), entity.0), world);
widget_system.set_last_change_tick(old_tick);


// later on after all the entities have been processed:
for system in context.systems.values_mut() {
    system.set_last_change_tick(world.read_change_tick());
}

Changelog

  • Added get_last_change_tick and set_last_change_tick to System's.

@StarArawn
Copy link
Contributor Author

Related to: #5633

@@ -106,6 +108,15 @@ impl<SystemA: System, SystemB: System<In = SystemA::Out>> System for ChainSystem
self.system_a.check_change_tick(change_tick);
self.system_b.check_change_tick(change_tick);
}

fn get_last_change_tick(&self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that if we can't implement these functions for every System, they aren't System functions.
That being said, it seems like chained systems should just assume a shared change tick? At the very least, setting last_change_tick for both seems correct.

@@ -224,6 +224,15 @@ impl System for FixedTimestep {
fn check_change_tick(&mut self, change_tick: u32) {
self.internal_system.check_change_tick(change_tick);
}

fn get_last_change_tick(&self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Why can't we just pipe through the internal system's last_change_tick?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Aug 30, 2022
@alice-i-cecile
Copy link
Member

@DJMcNab, would this benefit from the removal of chained systems as a hardcoded object?

@StarArawn StarArawn requested a review from cart August 30, 2022 21:12
@cart
Copy link
Member

cart commented Aug 30, 2022

@DJMcNab, would this benefit from the removal of chained systems as a hardcoded object?

I think it would just (effectively) implicitly do what @StarArawn did in the latest commit.

/// Allows users to get the system's last change tick.
fn get_last_change_tick(&self) -> u32;
/// Allows users to set the system's last change tick.
fn set_last_change_tick(&mut self, last_change_tick: u32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #5633, I think this deserves some level of warning around the effects this will have on change detection / that setting it manually could break things if you aren't careful.

@@ -59,6 +60,10 @@ pub trait System: Send + Sync + 'static {
fn default_labels(&self) -> Vec<SystemLabelId> {
Vec::new()
}
/// Allows users to get the system's last change tick.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Users" in this context is implicit. I think this should just say something like "Gets the system's last change tick". Same for the set method.

@DJMcNab
Copy link
Member

DJMcNab commented Aug 30, 2022

So the context here is running the same system multiple times within a single 'logical step'? I think in terms of chain system, this should be equivalent. I'm a little bit unsure why it can't just fall call the child's versions. Making chain system be a normal system would fix that. I've just now tried to resolve chain system, and it's still blocked on TAIT (.after breaks just using a naïve Box<dyn System>).

I'm tempted to say that the correct fix here is to make systems not track their own change tick at all, and always take it as an argument. That is, make the semantics be run_between_change_ticks. That being said, manual handling of change ticks in general seems like a very easy thing to get wrong, and I'm hesitant to support it at all.

Then again, we don't support use case cases where people run systems on their own outside of schedules, so if people want to have their fun, why not let them.
I'm hesistant to forward this guarantee onwards. That is, I'm happy enough if we merge this, but I don't want us to track this regressing somehow (e.g. a SystemParam can still assume that change ticks behave 'as-if' they were called from a schedule, with all the consequences thereof)

@StarArawn
Copy link
Contributor Author

I'm tempted to say that the correct fix here is to make systems not track their own change tick at all, and always take it as an argument. That is, make the semantics be run_between_change_ticks. That being said, manual handling of change ticks in general seems like a very easy thing to get wrong, and I'm hesitant to support it at all.

This was the first idea I had however it felt a bit more breaking and I wanted to limit my scope here as much as possible.

@StarArawn StarArawn requested a review from cart August 30, 2022 23:36
@cart
Copy link
Member

cart commented Aug 31, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 31, 2022
)

# Objective
I'm build a UI system for bevy. In this UI system there is a concept of a system per UI entity. I had an issue where change detection wasn't working how I would expect and it's because when a function system is ran the `last_change_tick` is updated with the latest tick(from world). In my particular case I want to "wait" to update the `last_change_tick` until after my system runs for each entity.

## Solution
Initially I thought bypassing the change detection all together would be a good fix, but on talking to some users in discord a simpler fix is to just expose `last_change_tick` to the end users. This is achieved by adding the following to the `System` trait:
```rust
    /// Allows users to get the system's last change tick.
    fn get_last_change_tick(&self) -> u32;
    /// Allows users to set the system's last change tick.
    fn set_last_change_tick(&mut self, last_change_tick: u32);
```

This causes a bit of weirdness with two implementors of `System`. `FixedTimestep` and `ChainSystem` both implement system and thus it's required that some sort of implementation be given for the new functions. I solved this by outputting a warning and not doing anything for these systems. 

I think it's important to understand why I can't add the new functions only to the function system and not to the `System` trait. In my code I store the systems generically as `Box<dyn System<...>>`. I do this because I have differing parameters that are being passed in depending on the UI widget's system.  As far as I can tell there isn't a way to take a system trait and cast it into a specific type without knowing what those parameters are.

In my own code this ends up looking something like:
```rust
// Runs per entity.
let old_tick = widget_system.get_last_change_tick();
should_update_children = widget_system.run((widget_tree.clone(), entity.0), world);
widget_system.set_last_change_tick(old_tick);


// later on after all the entities have been processed:
for system in context.systems.values_mut() {
    system.set_last_change_tick(world.read_change_tick());
}
```

## Changelog

- Added `get_last_change_tick` and `set_last_change_tick` to `System`'s.
@bors bors bot changed the title Added the ability to get or set the last change tick of a system. [Merged by Bors] - Added the ability to get or set the last change tick of a system. Aug 31, 2022
@bors bors bot closed this Aug 31, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…vyengine#5838)

# Objective
I'm build a UI system for bevy. In this UI system there is a concept of a system per UI entity. I had an issue where change detection wasn't working how I would expect and it's because when a function system is ran the `last_change_tick` is updated with the latest tick(from world). In my particular case I want to "wait" to update the `last_change_tick` until after my system runs for each entity.

## Solution
Initially I thought bypassing the change detection all together would be a good fix, but on talking to some users in discord a simpler fix is to just expose `last_change_tick` to the end users. This is achieved by adding the following to the `System` trait:
```rust
    /// Allows users to get the system's last change tick.
    fn get_last_change_tick(&self) -> u32;
    /// Allows users to set the system's last change tick.
    fn set_last_change_tick(&mut self, last_change_tick: u32);
```

This causes a bit of weirdness with two implementors of `System`. `FixedTimestep` and `ChainSystem` both implement system and thus it's required that some sort of implementation be given for the new functions. I solved this by outputting a warning and not doing anything for these systems. 

I think it's important to understand why I can't add the new functions only to the function system and not to the `System` trait. In my code I store the systems generically as `Box<dyn System<...>>`. I do this because I have differing parameters that are being passed in depending on the UI widget's system.  As far as I can tell there isn't a way to take a system trait and cast it into a specific type without knowing what those parameters are.

In my own code this ends up looking something like:
```rust
// Runs per entity.
let old_tick = widget_system.get_last_change_tick();
should_update_children = widget_system.run((widget_tree.clone(), entity.0), world);
widget_system.set_last_change_tick(old_tick);


// later on after all the entities have been processed:
for system in context.systems.values_mut() {
    system.set_last_change_tick(world.read_change_tick());
}
```

## Changelog

- Added `get_last_change_tick` and `set_last_change_tick` to `System`'s.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…vyengine#5838)

# Objective
I'm build a UI system for bevy. In this UI system there is a concept of a system per UI entity. I had an issue where change detection wasn't working how I would expect and it's because when a function system is ran the `last_change_tick` is updated with the latest tick(from world). In my particular case I want to "wait" to update the `last_change_tick` until after my system runs for each entity.

## Solution
Initially I thought bypassing the change detection all together would be a good fix, but on talking to some users in discord a simpler fix is to just expose `last_change_tick` to the end users. This is achieved by adding the following to the `System` trait:
```rust
    /// Allows users to get the system's last change tick.
    fn get_last_change_tick(&self) -> u32;
    /// Allows users to set the system's last change tick.
    fn set_last_change_tick(&mut self, last_change_tick: u32);
```

This causes a bit of weirdness with two implementors of `System`. `FixedTimestep` and `ChainSystem` both implement system and thus it's required that some sort of implementation be given for the new functions. I solved this by outputting a warning and not doing anything for these systems. 

I think it's important to understand why I can't add the new functions only to the function system and not to the `System` trait. In my code I store the systems generically as `Box<dyn System<...>>`. I do this because I have differing parameters that are being passed in depending on the UI widget's system.  As far as I can tell there isn't a way to take a system trait and cast it into a specific type without knowing what those parameters are.

In my own code this ends up looking something like:
```rust
// Runs per entity.
let old_tick = widget_system.get_last_change_tick();
should_update_children = widget_system.run((widget_tree.clone(), entity.0), world);
widget_system.set_last_change_tick(old_tick);


// later on after all the entities have been processed:
for system in context.systems.values_mut() {
    system.set_last_change_tick(world.read_change_tick());
}
```

## Changelog

- Added `get_last_change_tick` and `set_last_change_tick` to `System`'s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants